Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add a logout function #595

Merged
merged 6 commits into from
Nov 30, 2023
Merged

feat: add a logout function #595

merged 6 commits into from
Nov 30, 2023

Conversation

travis
Copy link
Member

@travis travis commented Nov 29, 2023

This resets the store, deleting the local agent's private keys and saved delegations. This is better than the previous implementation, and is appropriate for use cases like using console on a public or shared computer.

This PR also has a small tweak to replace JSX.Element with ReactNode, which I've found works better in downstream applications.

I've found this is more flexible and more idiomatic for components intended to be used in React applications (vs generic JSX applications, which imho this package should not strive to support)
this resets the store, which isn't exposed in the client's types. I think it's a little better to manage the store manually anyway since we're going to be calling a method on it directly, so I'm happy with this but could be dissuaded.
@travis travis changed the title Feat/logout feat: add a logout function Nov 29, 2023
Previously it was possible (and common in the case where the user hit a page to log out) for store to be undefined, because setupClient had not yet run. This actively loads the store before calling reset, which is much more reliable.
@travis travis marked this pull request as ready for review November 29, 2023 22:36
@travis travis requested review from alanshaw and gobengo November 29, 2023 22:42
}

useEffect(() => { void getClient() }, []) // load client - once.
useEffect(() => { void setupClient() }, []) // load client - once.
Copy link
Contributor

@gobengo gobengo Nov 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if props.connection changes, wouldn't we want this to run again? so that a new client gets created and passed to setClient?

maybe setupClient assignment should use React.useCallback with appropriate dependencies, and this useEffect can depend on the variables it depends on e.g. setupClient

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also maybe we should have the lint rule for react hook deps? https://github.com/facebook/react/blob/main/packages/eslint-plugin-react-hooks/README.md

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, I think you're right! I think that's orthogonal to the purpose of this PR though - happy to file an issue and even work on it today, but want to get this PR merged and shipped to fix the regression in production

}

useEffect(() => { void getClient() }, []) // load client - once.
useEffect(() => { void setupClient() }, []) // load client - once.
Copy link
Contributor

@gobengo gobengo Nov 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
useEffect(() => { void setupClient() }, []) // load client - once.
useEffect(() => { void setupClient() }, [servicePrincipal, connection, setupClient])

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmmm, not sure about this - on at the very least needs an update to the comment to explain why we're doing something so unusual, and I'm pretty sure this would trigger the linter you suggested above (though tbh I'm not a huge fan of that linter since it is not always correct and triggers on some valid use-cases)

Copy link
Contributor

@gobengo gobengo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consider this suggestion https://github.com/web3-storage/w3ui/pull/595/files#r1411076680
but IMO fine to address my comment in another issue

@travis travis merged commit 0995fd5 into main Nov 30, 2023
3 checks passed
@travis travis deleted the feat/logout branch November 30, 2023 18:15
travis pushed a commit that referenced this pull request Nov 30, 2023
🤖 I have created a release *beep* *boop*
---


##
[1.1.0](core-v1.0.1...core-v1.1.0)
(2023-11-30)


### Features

* add a logout function
([#595](#595))
([0995fd5](0995fd5))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
travis added a commit that referenced this pull request Nov 30, 2023
🤖 I have created a release *beep* *boop*
---


##
[1.1.0](react-v1.0.1...react-v1.1.0)
(2023-11-30)


### Features

* add a logout function
([#595](#595))
([0995fd5](0995fd5))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Travis Vachon <[email protected]>
travis pushed a commit that referenced this pull request Nov 30, 2023
🤖 I have created a release *beep* *boop*
---


##
[1.2.0](react-v1.1.1...react-v1.2.0)
(2023-11-30)


### Features

* add a logout function
([#595](#595))
([0995fd5](0995fd5))
* adds space-finder autocomplete combobox
([#268](#268))
([3dcd647](3dcd647))
* allow users to set page size in W3APIProvider
([#308](#308))
([814a293](814a293))
* club tropical w3 auth boxen
([#350](#350))
([2266eb2](2266eb2))
* Customizable UI components
([#208](#208))
([0a776fe](0a776fe))
* implement reverse paging
([#381](#381))
([10f059a](10f059a))
* Improve upload component flow
([#285](#285))
([ba9a3bf](ba9a3bf))
* simplify ([#591](#591))
([d1dfdf0](d1dfdf0))
* Storybook story improvements
([#294](#294))
([e0de2cc](e0de2cc))


### Bug Fixes

* fix w3console styling
([#320](#320))
([74a298c](74a298c))
* homepage URL in package.json
([1229119](1229119))
* remove authenticator class when registed
([#352](#352))
([3668f3b](3668f3b))
* w3console polish
([#284](#284))
([9a67365](9a67365))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
travis added a commit to storacha/console that referenced this pull request Dec 1, 2023
travis added a commit that referenced this pull request Dec 4, 2023
One of the core goals here was to fix a bug where the client wouldn't
get reset when servicePrincipal or connection changed. The easiest way
to make sure this was tested was to extract all of the datamodel
creation and management logic to a dedicated hook and test that.

Thanks to @gobengo for pointing out the bug and suggesting the fix:
#595 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants